SQL-2288: Create mongosqltranslate integration tests #251

EthanHardyMongo · 2024-10-14T20:41:48Z

This PR corrects the changes made in SQL-2291 and adds integration tests.

The path for libmongosqltranslate will instead be modified in this follow up ticket: https://jira.mongodb.org/browse/SQL-2403

Evergreen patch with everything passing: https://spruce.mongodb.com/version/670ee37a1d7c8900078da0ec/tasks?page=0&sorts=STATUS%3AASC%3BBASE_STATUS%3ADESC&statuses=success&variant=%5Eubuntu2204%24

…b#245) * cargo.lock * handle enterprise mode in get_next_metadata()

EthanHardyMongo · 2024-10-15T16:48:26Z

core/src/conn.rs

-        let command = CheckDriverVersion::new(DRIVER_ODBC_VERSION.clone());
+        let command = CheckDriverVersion::new(DRIVER_METRICS_VERSION.clone());


The DRIVER_ODBC_VERSION wasn't a valid semver version, so I switched to the DRIVER_METRICS_VERSION.

EthanHardyMongo · 2024-10-15T16:51:21Z

core/src/fields.rs

+
+                        let current_col_metadata_response: ResultSetSchema = if mongo_connection


This code was merged into master with SQL-2374

EthanHardyMongo · 2024-10-15T16:52:04Z

core/src/mongosqltranslate.rs

+#[serde(tag = "command_type")]
 pub enum CommandResponse {


The deserialization wasn't working without this.

EthanHardyMongo · 2024-10-15T16:55:00Z

core/src/mongosqltranslate.rs

+pub fn load_mongosqltranslate_library(mock_library: bool) {
    INIT.call_once(|| {
-        let library_path = if cfg!(test) {
+        let library_path = if mock_library {
            get_mock_library_path()
        } else {
-            get_library_path(LIBRARY_NAME)
+            get_library_path()


The mock_library is now only used by the unit test in this file; all the other tests can use the real library now, so I figured we could do this instead to make things easier.

evergreen.yml

EthanHardyMongo · 2024-10-15T17:10:37Z

integration_test/tests/odbc_2_tests.rs

+    ///
+    /// NOTE: FLAKY TEST


As I was testing, I noticed sometimes this test would fail, so I'm guessing it's flaky. Has anyone else noticed this?

No I haven't noticed it. What was the error it was reporting?

you can see in this patch that it failed. I'm not sure what the error is, but the exit code is 101, so I'm guessing something panicked.

Additionally, I just remembered, once I commented out that test and ran another patch, some other tests started failing with exit code 101. You can see in this patch. However, this is the only time these tests failed. Also, later on when I uncommented the test_type_listing test, it worked fine.

So I'm not really sure what's going on.

EthanHardyMongo · 2024-10-15T17:19:11Z

integration_test/tests/mongosqltranslate_tests.rs

+    #[test]
+    fn test_srv_style_uri_connection() {


This test requires libmongosqltranslate, so I moved it here to make testing easier.

constants/Cargo.toml

bucaojit

Lots of good work. I had some suggestions.

bucaojit · 2024-10-17T00:03:25Z

core/src/databases.rs

@@ -221,7 +221,13 @@ impl MongoDatabases {
            })
            .unwrap()
            .iter()
-            .filter(|&db_name| !db_name.is_empty() && !db_name.eq("admin"))
+            .filter(|&db_name| {


Since this list of databases and filter is in a couple places, can we add a helper function to filter out the databases we don't want?

bucaojit · 2024-10-17T00:10:41Z

core/src/fields.rs

+                            if let Err(error) = result_set_schema {
+                                // If there is an Error while deserializing the schema, we won't show any columns for it
+                                warnings.push(error);
+                                continue;
+                            }
+
+                            result_set_schema.unwrap()


How about something like this to avoid the unwrap, it has similar behavior:

match result_set_schema { Ok(schema) => schema, Err(error) => { warnings.push(error); continue; } }

bucaojit · 2024-10-17T00:12:57Z

integration_test/tests/odbc_2_tests.rs

+    ///
+    /// NOTE: FLAKY TEST


No I haven't noticed it. What was the error it was reporting?

bucaojit · 2024-10-17T00:20:55Z

integration_test/tests/mongosqltranslate_tests.rs

+            query.push(0);
+
+            assert_eq!(
+                SqlReturn::ERROR,


It would be good to add a check for what error we're getting here. If this one is SchemaDocumentNotFoundInSchemaCollection then it would be something like:

let error_message = get_sql_diagnostics(HandleType::SQL_HANDLE_STMT, stmt as Handle); assert!( error_message.contains("The following collection(s) were not found in the `__sql_schemas` collection"), "Expected error message about missing schema collection, got: {}", error_message ); ```

bucaojit · 2024-10-17T00:21:41Z

integration_test/tests/mongosqltranslate_tests.rs

+            query.push(0);
+
+            assert_eq!(
+                SqlReturn::ERROR,


Same comment here to confirm what error we get.

bucaojit · 2024-10-17T00:29:28Z

core/src/fields.rs

+                                    db.run_command(get_schema_cmd).await.unwrap(),
+                                )
+                                .map_err(|e| {
+                                    Error::CollectionDeserialization(collection_name.clone(), e)


How about an integration test that confirms we get this on a deserialization error?

bucaojit · 2024-10-17T00:31:08Z

integration_test/tests/mongosqltranslate_tests.rs

+        }
+        let _ = unsafe { Box::from_raw(env_handle) };
+    }
+}


One test I see in the design document that I don't see implemented yet is Invalid Command.

The Invalid Command tests ended up being unit tests in mongosqltranslate. Since we control the API and the driver's calls to the API, I don't think it's possible to have an integration test that has an invalid command.

EthanHardyMongo · 2024-10-17T21:12:43Z

core/src/col_metadata.rs

 // Struct representing the ResultSetSchema.
+// The `schema` field needs the alias `result_set_schema` because this struct is used to get the schema
+// from the __sql_schemas collection, which stores the schema in it's `schema` field, and the libmongosqltranslate
+// `translate` command, which stores the schema in it's `result_set_schema` field.
 #[derive(Serialize, Deserialize, PartialEq, Debug, Clone, Default)]
 pub struct ResultSetSchema {
-    #[serde(rename = "result_set_schema")]
+    #[serde(alias = "result_set_schema")]
    pub schema: crate::json_schema::Schema,


I'm not sure why this wasn't failing earlier, but I fixed it and added a comment explaining what's going on.

Oh actually I think it's because the error it would cause gets pushed into a warnings vector instead of causing a crash. This is probably why my tests weren't failing.

let result_set_schema: Result<ResultSetSchema> = ResultSetSchema::from_sql_schemas_document(&schema_doc).map_err( |e| { Error::CollectionDeserialization(collection_name.clone(), e) }, ); match result_set_schema { Ok(result_set_schema) => result_set_schema, Err(error) => { // If there is an Error while deserializing the schema, we won't show any columns for it warnings.push(error); continue; } }

Oh wait never mind. The part of the code that would fail was never called by my tests. That's why this wasn't causing any failures.

EthanHardyMongo and others added 19 commits March 6, 2024 16:51

cargo.lock

91e709e

Merge remote-tracking branch 'upstream/master'

f9039e0

Merge remote-tracking branch 'upstream/master'

2997ffd

Merge remote-tracking branch 'upstream/master'

71e5ec3

Merge remote-tracking branch 'upstream/master'

6519307

Merge remote-tracking branch 'upstream/master'

914ca8f

Merge remote-tracking branch 'upstream/master'

59a37de

Merge remote-tracking branch 'upstream/master'

74b8d02

Merge remote-tracking branch 'upstream/master'

e7ce323

Merge remote-tracking branch 'upstream/master'

401713b

Merge remote-tracking branch 'upstream/master'

65ff048

Merge remote-tracking branch 'upstream/master'

5bc7a32

Merge remote-tracking branch 'upstream/master'

f5541e3

Merge remote-tracking branch 'upstream/master'

9125f5c

Merge remote-tracking branch 'upstream/master'

8050d4f

Merge remote-tracking branch 'upstream/master'

4e45044

SQL-2374: Implement columns metadata logic for Direct cluster (mongod…

52be409

…b#245) * cargo.lock * handle enterprise mode in get_next_metadata()

Merge remote-tracking branch 'upstream/master'

558358d

create mongosqltranslate tests and fix logic

e7e8234

EthanHardyMongo marked this pull request as ready for review October 14, 2024 20:45

EthanHardyMongo added 3 commits October 15, 2024 12:55

fix evergreen names

ac45b5d

remove test flag

2ca83d0

rename evergreen mongosqltranslate task

d3645cf

EthanHardyMongo commented Oct 15, 2024

View reviewed changes

EthanHardyMongo added 2 commits October 15, 2024 15:28

change version back to 0.0.0

2f9c330

change mongosqltranslate library used in evergreen

faba35f

EthanHardyMongo requested review from nbagnard and bucaojit October 15, 2024 21:48

bucaojit requested changes Oct 17, 2024

View reviewed changes

respond to some feedback

b17ce9e

EthanHardyMongo commented Oct 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SQL-2288: Create mongosqltranslate integration tests #251

SQL-2288: Create mongosqltranslate integration tests #251

EthanHardyMongo commented Oct 14, 2024 •

edited

Loading

EthanHardyMongo Oct 15, 2024

EthanHardyMongo Oct 15, 2024

EthanHardyMongo Oct 15, 2024

EthanHardyMongo Oct 15, 2024

EthanHardyMongo Oct 15, 2024

bucaojit Oct 17, 2024

EthanHardyMongo Oct 17, 2024

EthanHardyMongo Oct 15, 2024

bucaojit left a comment

bucaojit Oct 17, 2024

bucaojit Oct 17, 2024

bucaojit Oct 17, 2024

bucaojit Oct 17, 2024

bucaojit Oct 17, 2024

bucaojit Oct 17, 2024

bucaojit Oct 17, 2024

EthanHardyMongo Oct 17, 2024

EthanHardyMongo Oct 17, 2024

EthanHardyMongo Oct 17, 2024

EthanHardyMongo Oct 17, 2024

		let command = CheckDriverVersion::new(DRIVER_ODBC_VERSION.clone());
		let command = CheckDriverVersion::new(DRIVER_METRICS_VERSION.clone());


		let current_col_metadata_response: ResultSetSchema = if mongo_connection

SQL-2288: Create mongosqltranslate integration tests #251

Are you sure you want to change the base?

SQL-2288: Create mongosqltranslate integration tests #251

Conversation

EthanHardyMongo commented Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bucaojit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EthanHardyMongo commented Oct 14, 2024 •

edited

Loading